Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always use read-object hook during checkout #92

Merged
merged 3 commits into from
Aug 28, 2019
Merged

Always use read-object hook during checkout #92

merged 3 commits into from
Aug 28, 2019

Conversation

derrickstolee
Copy link
Contributor

While refactoring the CloneVerb in #77, I moved the git checkout -f command to the end of clone, as needed. This is because we will actually fill out blobs during this initial checkout.

However, there was one slight problem: we need the read-object hook. There are two ways to do a checkout in the GitProcess, and one was not allowing the read-object hook.

Before #77, this checkout was called before the mount was ready, but that's no longer important.

Thanks @jamill for finding this bug!

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee derrickstolee added the bug correctness issues label Aug 28, 2019
@derrickstolee derrickstolee added this to the Demo milestone Aug 28, 2019
@derrickstolee
Copy link
Contributor Author

This didn't repro in any of my local tests because we had the blobs in the shared object cache.

Copy link
Member

@jamill jamill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to write a test for this scenario to prevent future regressions?

@jamill
Copy link
Member

jamill commented Aug 28, 2019

Also - would it be possible to generate more diagnostic information around this workflow (or maybe that is just part of the work to flesh out the workflow)... It looks like we just write out that an error occurred, maybe there was some output on stderr that we could log.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
@derrickstolee
Copy link
Contributor Author

Is it possible to write a test for this scenario to prevent future regressions?

I think the proper test is to create a new, sparse clone using a local object cache. That prevents previous tests that ran a full prefetch from making the objects available. I'm going to try this out, but clone tests are particularly tricky (when they fail, the clone logs don't get output by the functional test machinery since the new clone enlistment is outside of the enlistment set up by the base class).

Also - would it be possible to generate more diagnostic information around this workflow (or maybe that is just part of the work to flesh out the workflow)... It looks like we just write out that an error occurred, maybe there was some output on stderr that we could log.

The latest commit adds the Git output and errors to the clone log file, but not to the user.

@jamill
Copy link
Member

jamill commented Aug 28, 2019

Thanks for the quick turnaround! I think the extra logging will be helpful if we hit an error on this part of clone in the future

[TestCase]
public void SparseCloneWithNoPrefetchSucceeds()
{
string newEnlistmentRoot = ScalarFunctionalTestEnlistment.GetUniqueEnlistmentRoot();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you call ScalarFunctionalTestEnlistment.CloneAndMountWithPerRepoCache here instead of manually setting up the clone?

ProcessResult result = ProcessHelper.Run(processInfo);
result.ExitCode.ShouldEqual(0);

result = GitProcess.InvokeProcess(Path.Combine(newEnlistmentRoot, "src"), "status");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're able to use CloneAndMountWithPerRepoCache you could use enlistment.RepoRoot here instead of Path.Combine(newEnlistmentRoot, "src")

Copy link
Member

@wilbaker wilbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with suggestions

ScalarFunctionalTestEnlistment enlistment = ScalarFunctionalTestEnlistment.CloneAndMountWithPerRepoCache(ScalarTestConfig.PathToScalar, skipPrefetch: true);

ProcessResult result = GitProcess.InvokeProcess(enlistment.RepoRoot, "status");
result.ExitCode.ShouldEqual(0, result.Errors);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should call UnmountAndDeleteAll at the end here (or in a try/finally)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to clarify, enlistment.UnmountAndDeleteAll (and apologies for missing that with the earlier suggestion)

Signed-off-by: Derrick Stolee <stolee@gmail.com>
Copy link
Member

@wilbaker wilbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest commit looks good!

@derrickstolee
Copy link
Contributor Author

Latest commit looks good!

Thanks for the help with these tests! Unfamiliar territory for me here. :D

@derrickstolee derrickstolee merged commit bdd2a24 into microsoft:master Aug 28, 2019
@derrickstolee derrickstolee deleted the fix-checkout-on-clone branch November 18, 2019 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug correctness issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants